Skip to content

Conversation

@wjyrich
Copy link
Contributor

@wjyrich wjyrich commented Jun 17, 2025

as title.

PMS-BUG-309173

Summary by Sourcery

Implement smart-hide on desktop-hide actions by monitoring window state transitions and updating dock visibility accordingly

New Features:

  • Track and update window hidden state on _NET_WM_STATE changes (including Win+D desktop hide) to trigger smart-hide behavior

Enhancements:

  • Always treat minimized or hidden windows as non-overlapping with the dock area

@sourcery-ai
Copy link

sourcery-ai bot commented Jun 17, 2025

Reviewer's Guide

This PR implements smart hiding by monitoring _NET_WM_STATE changes (including empty state for Win+D), updating each window’s minimized/hidden status accordingly, and ensuring hidden or minimized windows do not overlap the dock.

Sequence diagram for smart window hiding on _NET_WM_STATE changes

sequenceDiagram
    participant X11DockHelper
    participant XcbHelper
    participant WindowData
    participant Dock
    actor User

    User->>X11DockHelper: Triggers Win+D or window state change
    X11DockHelper->>XcbHelper: getAtomByName("_NET_WM_STATE")
    X11DockHelper->>XcbHelper: getWindowState(window)
    XcbHelper-->>X11DockHelper: Returns window state atoms
    X11DockHelper->>WindowData: Update isMinimized based on state (HIDDEN, BELOW, SKIP_TASKBAR, or empty)
    X11DockHelper->>X11DockHelper: updateWindowHideState(window)
    X11DockHelper->>WindowData: Set overlap = false if minimized/hidden
    WindowData-->>Dock: No overlap with dock if hidden/minimized
Loading

Class diagram for updated window state and overlap logic

classDiagram
    class X11DockHelper {
        +void onWindowPropertyChanged(xcb_window_t, xcb_atom_t)
        +void updateWindowHideState(xcb_window_t)
    }
    class XcbHelper {
        +QList<xcb_atom_t> getWindowState(xcb_window_t)
        +xcb_atom_t getAtomByName(QString)
    }
    class WindowData {
        bool isMinimized
        QRect rect
        bool overlap
    }
    X11DockHelper --> XcbHelper : uses
    X11DockHelper --> WindowData : updates
    WindowData --> X11DockHelper : overlap updated
    XcbHelper <.. X11DockHelper : queried for atoms and state
Loading

File-Level Changes

Change Details Files
Extend property change handler to react to desktop hide events
  • Plug into _NET_WM_STATE property changes
  • Fetch current window state atoms
panels/dock/x11dockhelper.cpp
Determine hidden/minimized status from state flags (including empty state) and trigger updates
  • Detect HIDDEN, BELOW, SKIP_TASKBAR or no atoms as hidden
  • Compare new hidden flag against previous minimized status
  • Invoke updateWindowHideState on state transitions
panels/dock/x11dockhelper.cpp
Prevent minimized/hidden windows from overlapping the dock
  • In updateWindowHideState, force overlap=false when minimized
panels/dock/x11dockhelper.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @wjyrich - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

  • Undefined variable 'states' used here (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `panels/dock/x11dockhelper.cpp:442` </location>
<code_context>
+            m_windows[window]->isMinimized = m_xcbHelper->getWindowState(window).contains(m_xcbHelper->getAtomByName("_NET_WM_STATE_HIDDEN"));
+            // 检查窗口是否被隐藏
+            // Win+D时窗口状态会被清空,这也表示窗口被隐藏
+            bool isWindowHidden = states.contains(m_xcbHelper->getAtomByName("_NET_WM_STATE_HIDDEN")) ||
+                                states.contains(m_xcbHelper->getAtomByName("_NET_WM_STATE_BELOW")) ||
+                                states.contains(m_xcbHelper->getAtomByName("_NET_WM_STATE_SKIP_TASKBAR")) ||
</code_context>

<issue_to_address>
Undefined variable 'states' used here

`states` is used without being defined. Please assign the result of `m_xcbHelper->getWindowState(window)` to a local variable named `states` before referencing it.
</issue_to_address>

### Comment 2
<location> `panels/dock/x11dockhelper.cpp:439` </location>
<code_context>
             onWindowWorkspaceChanged(window);
+        } else if (atom == m_xcbHelper->getAtomByName("_NET_WM_STATE")) {
+            bool wasMinimized = m_windows[window]->isMinimized;
+            m_windows[window]->isMinimized = m_xcbHelper->getWindowState(window).contains(m_xcbHelper->getAtomByName("_NET_WM_STATE_HIDDEN"));
+            // 检查窗口是否被隐藏
+            // Win+D时窗口状态会被清空,这也表示窗口被隐藏
</code_context>

<issue_to_address>
Initial isMinimized assignment is overwritten

Compute `isWindowHidden` first, then assign `isMinimized` once to avoid redundant assignments.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Member

@BLumia BLumia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果不是本阶段必解的话,应该改监听 org.kde.KWinShowingDesktopStateChanged

@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 代码注释

    • updateWindowHideState函数中添加注释说明最小化窗口不会与任务栏重叠,有助于其他开发者理解代码逻辑。
  2. 变量命名

    • m_showingDesktop变量名清晰,易于理解其用途。
  3. 代码结构

    • setupKWinDBusConnection函数和onShowingDesktopChanged函数的添加使得代码结构更加清晰,将显示桌面状态的逻辑分离出来,提高了代码的可维护性。
  4. 信号发射

    • onShowingDesktopChanged函数中,当显示桌面状态变化时,通过Q_EMIT发射isWindowOverlapChanged信号,这是正确的做法,但需要确保所有相关的槽函数都已经连接到这个信号。
  5. D-Bus连接

    • setupKWinDBusConnection函数中,连接到KWin的D-Bus接口,监听showingDesktopChanged信号。这是一个好的实践,因为它允许X11DockHelper与KWin进行通信,以获取显示桌面状态。
  6. 性能考虑

    • isWindowOverlap函数中,使用std::for_each遍历窗口列表,这是一个简单且有效的方法。但是,如果窗口列表非常大,可以考虑使用更高效的数据结构或算法来提高性能。
  7. 代码重复

    • updateWindowHideState函数中,oldOverlap变量用于存储窗口重叠状态的前一个值,但在最小化窗口的情况下,这个值被直接设置为false。这可能是一个逻辑错误,因为最小化窗口的状态变化不应该影响oldOverlap的值。
  8. 安全性

    • 代码中没有明显的安全漏洞,但是需要确保与KWin的D-Bus通信是安全的,并且不会受到中间人攻击。

总体来说,代码的改动提高了代码的可读性和可维护性,并且添加了新的功能来处理显示桌面状态。但是,需要进一步确认最小化窗口状态变化对oldOverlap的影响是否正确。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, wjyrich

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@BLumia BLumia merged commit 9c723ee into linuxdeepin:master Jun 17, 2025
7 of 10 checks passed
"showingDesktopChanged",
this,
SLOT(onShowingDesktopChanged(bool))
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需要同步下m_showingDesktop的状态吧,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants